From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 42D9821BC6A7E for ; Mon, 27 Mar 2017 02:19:11 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 83CFF4E4D9; Mon, 27 Mar 2017 09:19:09 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 83CFF4E4D9 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 83CFF4E4D9 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-77.phx2.redhat.com [10.3.116.77]) by smtp.corp.redhat.com (Postfix) with ESMTP id EC94F17C46; Mon, 27 Mar 2017 09:19:06 +0000 (UTC) To: Brijesh Singh , michael.d.kinney@intel.com, jordan.l.justen@intel.com, edk2-devel@ml01.01.org, liming.gao@intel.com References: <149013076154.27235.10725020825643505862.stgit@brijesh-build-machine> <149013078709.27235.16179070664669554073.stgit@brijesh-build-machine> Cc: brijesh.singh@amd.com, leo.duran@amd.com, Thomas.Lendacky@amd.com From: Laszlo Ersek Message-ID: Date: Mon, 27 Mar 2017 11:19:05 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <149013078709.27235.16179070664669554073.stgit@brijesh-build-machine> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Mon, 27 Mar 2017 09:19:10 +0000 (UTC) Subject: Re: [RFC PATCH v2 04/10] OvmfPkg/BaseMemcryptSevLib: Add SEV helper library X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Mar 2017 09:19:11 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/21/17 22:13, Brijesh Singh wrote: > Add Secure Encrypted Virtualization (SEV) helper library. The library provides > the routines to set or clear memory encryption bit for a given memory region > and functions to check whether SEV is enabled. Please rewrap this commit message to 74 characters. > > Signed-off-by: Brijesh Singh > --- > OvmfPkg/Include/Library/MemEncryptSevLib.h | 69 +++++ > .../BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf | 46 +++ > .../BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 124 ++++++++ > .../BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 120 ++++++++ > .../BaseMemEncryptSevLib/X64/VirtualMemory.c | 304 ++++++++++++++++++++ > .../BaseMemEncryptSevLib/X64/VirtualMemory.h | 158 ++++++++++ > OvmfPkg/OvmfPkgIa32.dsc | 1 > OvmfPkg/OvmfPkgIa32X64.dsc | 1 > OvmfPkg/OvmfPkgX64.dsc | 1 Please format patches as described under https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23 In particular, the options --stat=1000 \ --stat-graph-width=20 \ will ensure that git does not use the "..." abbreviation in the diffstat, while also keeping the plus/minus stats reasonably narrow. > 9 files changed, 824 insertions(+) > create mode 100644 OvmfPkg/Include/Library/MemEncryptSevLib.h > create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf > create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c > create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h > > diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h > new file mode 100644 > index 0000000..a9e9356 > --- /dev/null > +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h > @@ -0,0 +1,69 @@ > +/** @file > + > + Define Secure Encrypted Virtualization (SEV) base library helper function > + > + Copyright (c) 2017, AMD Incorporated. All rights reserved.
> + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD License > + which accompanies this distribution. The full text of the license may be found at Overlong line. No line should be wider than 79 chars. > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#ifndef __MEM_ENCRYPT_SEV_LIB_H_ > +#define __MEM_ENCRYPT_SEV_LIB_H_ If you start the guard macro name with two underscores, then please also end it with two underscores. > + > +#include > + > +/** > + Returns a boolean to indicate whether SEV is enabled > + > + @retval TRUE When SEV is active > + @retval FALSE When SEV is not enabled > + **/ > +BOOLEAN > +EFIAPI > +MemEncryptSevIsEnabled ( > + VOID > + ); Would it make sense to call this library function in PlatformPei, rather than add a separate SevIsEnabled() function to it (in patch #3)? The implementations look nearly identical. The client module type restrictions under LIBRARY_CLASS (below) look fine for that; all of the permitted module types support writeable global variables in OVMF. > + > +/** > + This function clears memory encryption bit for the memory region specified by BaseAddress and > + Number of pages from the current page table context. > + > + @param[in] BaseAddress The physical address that is the start address of a memory region. > + @param[in] NumberOfPages The number of pages from start memory region. > + > + @retval RETURN_SUCCESS The attributes were cleared for the memory region. > + @retval RETURN_INVALID_PARAMETER Number of pages is zero. > + @retval RETURN_UNSUPPORTED Clearing memory encryption attribute is not supported > + **/ These lines as well are too wide. > +RETURN_STATUS > +EFIAPI > +MemEncryptSevClearPageEncMask ( > + IN PHYSICAL_ADDRESS BaseAddress, > + IN UINT32 NumberOfPages > + ); > + > +/** > + This function sets memory encryption bit for the memory region specified by BaseAddress and > + Number of pages from the current page table context. > + > + @param[in] BaseAddress The physical address that is the start address of a memory region. > + @param[in] NumberOfPages The number of pages from start memory region. > + > + @retval RETURN_SUCCESS The attributes were set for the memory region. > + @retval RETURN_INVALID_PARAMETER Number of pages is zero. > + @retval RETURN_UNSUPPORTED Clearing memory encryption attribute is not supported > + **/ > +RETURN_STATUS > +EFIAPI > +MemEncryptSevSetPageEncMask ( > + IN PHYSICAL_ADDRESS BaseAddress, > + IN UINT32 NumberOfPages > + ); > +#endif // __MEM_ENCRYPT_SEV_LIB_H_ > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf > new file mode 100644 > index 0000000..c23261f > --- /dev/null > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf > @@ -0,0 +1,46 @@ > +## @file Please add one or two sentences under @file about the purpose of this library instance. > +# > +# Copyright (c) 2017 Advanced Micro Devices. All rights reserved.
> +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +# > +## Please rewrap to 79 chars. (I think I won't ask for that explicitly in any other locations; please just ensure all new files conform to that.) > + > +[Defines] > + INF_VERSION = 0x00010005 Please use "1.25" here, that's the most recent release of the INF spec (to my knowledge). > + BASE_NAME = MemEncryptSevLib > + FILE_GUID = c1594631-3888-4be4-949f-9c630dbc842b > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = MemEncryptSevLib|PEIM DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_DRIVER > + > +# > +# The following information is for reference only and not required by the build tools. > +# > +# VALID_ARCHITECTURES = IA32 X64 > +# > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + OvmfPkg/OvmfPkg.dec > + UefiCpuPkg/UefiCpuPkg.dec > + > +[Sources.X64] > + X64/MemEncryptSevLib.c > + X64/VirtualMemory.c > + > +[Sources.IA32] > + Ia32/MemEncryptSevLib.c > + > +[LibraryClasses] > + BaseLib > + DebugLib > + MemoryAllocationLib > + CacheMaintenanceLib Please sort the entries in each section alphabetically. > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > new file mode 100644 > index 0000000..70fdd2e > --- /dev/null > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > @@ -0,0 +1,124 @@ > +/** @file > + > + Secure Encrypted Virtualization (SEV) library helper function > + > + Copyright (c) 2017, AMD Incorporated. All rights reserved.
> + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD License > + which accompanies this distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include "Uefi.h" > +#include > +#include > +#include > +#include > +#include > + > +STATIC BOOLEAN SevStatus = FALSE; > +STATIC BOOLEAN SevStatusChecked = FALSE; These variables should be called mSevStatus and mSevStatusChecked. > + > +/** > + > + Returns a boolean to indicate whether SEV is enabled > + > + @retval TRUE When SEV is active > + @retval FALSE When SEV is not enabled > + **/ > +BOOLEAN > +EFIAPI > +MemEncryptSevIsEnabled ( > + VOID > + ) > +{ > + UINT32 RegEax; > + MSR_SEV_STATUS_REGISTER Msr; > + CPUID_MEMORY_ENCRYPTION_INFO_EAX Eax; Please align the variable names. > + > + // > + // If Status is already checked then return it > + // > + if (SevStatusChecked) { > + return SevStatus; > + } > + > + // > + // 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) { > + // > + // Check MSR_0xC0010131 Bit 0 (Sev is Enabled) > + // > + Msr.Uint32 = AsmReadMsr32 (MSR_SEV_STATUS); > + if (Msr.Bits.SevBit) { You forgot to set mSevStatusChecked to TRUE here. > + return TRUE; > + } > + } > + } > + > + SevStatusChecked = TRUE; > + > + return FALSE; > +} > + > +/** > + This function clears memory encryption bit for the memory region specified by BaseAddress and > + Number of pages from the current page table context. > + > + @param[in] BaseAddress The physical address that is the start address of a memory region. > + @param[in] NumberOfPages The number of pages from start memory region. > + > + @retval RETURN_SUCCESS The attributes were cleared 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 > +MemEncryptSevClearPageEncMask ( > + IN PHYSICAL_ADDRESS BaseAddress, > + IN UINT32 NumberOfPages > + ) > +{ > + // > + // Memory encryption bit is not accessible in 32-bit mode > + // > + return RETURN_UNSUPPORTED; > +} > + > +/** > + This function sets memory encryption bit for the memory region specified by BaseAddress and > + Number of pages from the current page table context. > + > + @param[in] BaseAddress The physical address that is the start address of a memory region. > + @param[in] NumberOfPages The number of pages from start memory region. > + > + @retval RETURN_SUCCESS The attributes were cleared 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 > +MemEncryptSevSetPageEncMask ( > + IN PHYSICAL_ADDRESS BaseAddress, > + IN UINT32 NumberOfPages > + ) > +{ > + // > + // Memory encryption bit is not accessible in 32-bit mode > + // > + return RETURN_UNSUPPORTED; > +} > + > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > new file mode 100644 > index 0000000..098acf2 > --- /dev/null > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > @@ -0,0 +1,120 @@ > +/** @file > + > + Secure Encrypted Virtualization (SEV) library helper function > + > + Copyright (c) 2017, AMD Incorporated. All rights reserved.
> + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD License > + which accompanies this distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include "Uefi.h" > +#include > +#include > +#include > +#include > +#include > + > +#include "VirtualMemory.h" > + > +STATIC BOOLEAN SevStatus = FALSE; > +STATIC BOOLEAN SevStatusChecked = FALSE; > + > +/** > + Returns a boolean to indicate whether SEV is enabled > + > + @retval TRUE When SEV is active > + @retval FALSE When SEV is not enabled > + **/ > +BOOLEAN > +EFIAPI > +MemEncryptSevIsEnabled ( > + VOID > + ) > +{ > + UINT32 RegEax; > + MSR_SEV_STATUS_REGISTER Msr; > + CPUID_MEMORY_ENCRYPTION_INFO_EAX Eax; > + > + // > + // If Status is already checked then return it > + // > + if (SevStatusChecked) { > + return SevStatus; > + } > + > + // > + // 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) { > + // > + // Check MSR_0xC0010131 Bit 0 (Sev Enabled) > + // > + Msr.Uint32 = AsmReadMsr32 (MSR_SEV_STATUS); > + if (Msr.Bits.SevBit) { > + return TRUE; > + } > + } > + } > + > + SevStatusChecked = TRUE; > + > + return FALSE; > +} In fact, I suggest that: (a) you introduce two new files, called MemEncryptSevLibInternal.h MemEncryptSevLibInternal.c (b) add the .c file to both [Sources.X64] and [Sources.IA32], (c) rename MemEncryptSevIsEnabled() to InternalMemEncryptSevIsEnabled, and declare it in the new header file, (d) move MemEncryptSevIsEnabled, SevStatus and SevStatusChecked to the new .c file. > + > +/** > + > + This function clears memory encryption bit for the memory region specified by BaseAddress and > + Number of pages from the current page table context. > + > + @param[in] BaseAddress The physical address that is the start address of a memory region. > + @param[in] NumberOfPages The number of pages from start memory region. > + > + @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 encryption attribute is not supported > + **/ > +RETURN_STATUS > +EFIAPI > +MemEncryptSevClearPageEncMask ( > + IN PHYSICAL_ADDRESS BaseAddress, > + IN UINT32 NumPages > + ) > +{ > + return Set_Memory_Decrypted (BaseAddress, NumPages * EFI_PAGE_SIZE); > +} "Set_Memory_Decrypted" is not idiomatic edk2, please call it SetMemoryDecrypted, Also, the second argument should be written as EFI_PAGES_TO_SIZE (NumPages) ... However, for that, you should please switch the type of "NumPages" to UINTN. Is that functionally okay? > + > +/** > + > + This function clears memory encryption bit for the memory region specified by BaseAddress and > + Number of pages from the current page table context. > + > + @param[in] BaseAddress The physical address that is the start address of a memory region. > + @param[in] NumberOfPages The number of pages from start memory region. > + > + @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 encryption attribute is not supported > + **/ > +RETURN_STATUS > +EFIAPI > +MemEncryptSevSetPageEncMask ( > + IN PHYSICAL_ADDRESS BaseAddress, > + IN UINT32 NumPages > + ) > +{ > + return Set_Memory_Encrypted (BaseAddress, NumPages * EFI_PAGE_SIZE); > +} Same comments here. > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c > new file mode 100644 > index 0000000..7acebf3 > --- /dev/null > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c > @@ -0,0 +1,304 @@ > +/** @file > + > + Virtual Memory Management Services to set or clear the memory encryption bit > + > + References: > + 1) IA-32 Intel(R) Architecture Software Developer's Manual Volume 1:Basic Architecture, Intel > + 2) IA-32 Intel(R) Architecture Software Developer's Manual Volume 2:Instruction Set Reference, Intel > + 3) IA-32 Intel(R) Architecture Software Developer's Manual Volume 3:System Programmer's Guide, Intel > + > +Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
> +Copyright (c) 2017, AMD Incorporated. All rights reserved.
> + > +This program and the accompanying materials > +are licensed and made available under the terms and conditions of the BSD License > +which accompanies this distribution. The full text of the license may be found at > +http://opensource.org/licenses/bsd-license.php > + > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include "VirtualMemory.h" > + > +#include Please put the <> includes first, and the "" includes second. > + > +STATIC UINT64 AddressEncMask; Should be called "mAddressEncMask". > + > +typedef enum { > + SetCBit, > + ClearCBit > +} MAP_RANGE_MODE; > + > +/** > + Split 2M page to 4K. > + > + @param[in] PhysicalAddress Start physical address the 2M page covered. > + @param[in, out] PageEntry2M Pointer to 2M page entry. > + @param[in] StackBase Stack base address. > + @param[in] StackSize Stack size. > + > +**/ > +STATIC > +VOID > +Split2MPageTo4K ( > + IN EFI_PHYSICAL_ADDRESS PhysicalAddress, Your use of PHYSICAL_ADDRESS <-> EFI_PHYSICAL_ADDRESS is inconsistent. Since this is a BASE type library, I suggest to stick with PHYSICAL_ADDRESS. Can you please check all occurrences of EFI_PHYSICAL_ADDRESS? > + IN OUT UINT64 *PageEntry2M, IN and OUT columns should be visually separated across all parameters. > + IN EFI_PHYSICAL_ADDRESS StackBase, > + IN UINTN StackSize > + ) > +{ > + EFI_PHYSICAL_ADDRESS PhysicalAddress4K; > + UINTN IndexOfPageTableEntries; > + PAGE_TABLE_4K_ENTRY *PageTableEntry, *PageTableEntry1; > + > + PageTableEntry = AllocatePages(1); > + > + PageTableEntry1 = PageTableEntry; > + > + ASSERT (PageTableEntry != NULL); > + ASSERT (*PageEntry2M & AddressEncMask); > + > + PhysicalAddress4K = PhysicalAddress; > + for (IndexOfPageTableEntries = 0; IndexOfPageTableEntries < 512; IndexOfPageTableEntries++, PageTableEntry++, PhysicalAddress4K += SIZE_4KB) { > + // > + // Fill in the Page Table entries > + // > + PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask; > + PageTableEntry->Bits.ReadWrite = 1; > + PageTableEntry->Bits.Present = 1; > + if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + StackSize)) { > + // > + // Set Nx bit for stack. > + // > + PageTableEntry->Bits.Nx = 1; > + } > + } > + > + // > + // Fill in 2M page entry. > + // > + *PageEntry2M = (UINT64) (UINTN) PageTableEntry1 | IA32_PG_P | IA32_PG_RW | AddressEncMask; > +} Side question: is this function copied from some other place in edk2? If so, I suggest to add a comment (in the code or in the commit message) where it comes from. Also, shouldn't you call CpuFlushTlb() somewhere, when messing with page tables? (Hm, maybe not; a given virtual address will continue to map to the same physical address.) > + > +/** > + Split 1G page to 2M. > + > + @param[in] PhysicalAddress Start physical address the 1G page covered. > + @param[in, out] PageEntry1G Pointer to 1G page entry. > + @param[in] StackBase Stack base address. > + @param[in] StackSize Stack size. > + > +**/ > +STATIC > +VOID > +Split1GPageTo2M ( > + IN EFI_PHYSICAL_ADDRESS PhysicalAddress, > + IN OUT UINT64 *PageEntry1G, > + IN EFI_PHYSICAL_ADDRESS StackBase, > + IN UINTN StackSize > + ) > +{ > + EFI_PHYSICAL_ADDRESS PhysicalAddress2M; > + UINTN IndexOfPageDirectoryEntries; > + PAGE_TABLE_ENTRY *PageDirectoryEntry; > + > + PageDirectoryEntry = AllocatePages(1); > + > + ASSERT (PageDirectoryEntry != NULL); > + ASSERT (*PageEntry1G & AddressEncMask); > + // > + // Fill in 1G page entry. > + // > + *PageEntry1G = (UINT64) (UINTN) PageDirectoryEntry | IA32_PG_P | IA32_PG_RW | AddressEncMask; > + > + PhysicalAddress2M = PhysicalAddress; > + for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M += SIZE_2MB) { > + if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M + SIZE_2MB) > StackBase)) { > + // > + // Need to split this 2M page that covers stack range. > + // > + Split2MPageTo4K (PhysicalAddress2M, (UINT64 *) PageDirectoryEntry, StackBase, StackSize); > + } else { > + // > + // Fill in the Page Directory entries > + // > + PageDirectoryEntry->Uint64 = (UINT64) PhysicalAddress2M | AddressEncMask; > + PageDirectoryEntry->Bits.ReadWrite = 1; > + PageDirectoryEntry->Bits.Present = 1; > + PageDirectoryEntry->Bits.MustBe1 = 1; > + } > + } > +} > + > + > +STATIC VOID > +SetOrClearCBit( > + IN UINT64* PageTablePointer, > + IN MAP_RANGE_MODE Mode Non-idiomatic formatting, plus "IN" is actually incorrect for PageTablePointer, as it is both read and written. This is what you need: IN OUT UINT64 *PageTablePointer, IN MAP_RANGE_MODE Mode > + ) > +{ > + if (Mode == SetCBit) { > + *PageTablePointer |= AddressEncMask; > + } else { > + *PageTablePointer &= ~AddressEncMask; > + } > + > +} > + > +STATIC > +UINT64 > +GetMemEncryptionAddressMask ( > + VOID > + ) > +{ > + UINT64 MeMask; > + CPUID_MEMORY_ENCRYPTION_INFO_EBX Ebx; variable name alignment > + > + // > + // CPUID Fn8000_001F[EBX] Bit 0:5 (memory encryption bit position) > + // > + AsmCpuid (CPUID_MEMORY_ENCRYPTION_INFO, NULL, &Ebx.Uint32, NULL, NULL); > + MeMask = LShiftU64 (1, Ebx.Bits.PtePosBits); > + > + return MeMask & PAGING_1G_ADDRESS_MASK_64; > +} Can you perhaps export this function and call it from PlatformPei as well? Just asking. > + > +STATIC > +EFI_STATUS > +EFIAPI > +Set_Memory_Enc_Dec ( Please use camel case without underscores. Also, this function looks like the "workhorse" of the library. Can you add a leading comment block to it? > + IN EFI_PHYSICAL_ADDRESS PhysicalAddress, > + IN UINT64 Length, > + IN MAP_RANGE_MODE Mode > + ) > +{ > + PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry; > + PAGE_MAP_AND_DIRECTORY_POINTER *PageUpperDirectoryPointerEntry; > + PAGE_MAP_AND_DIRECTORY_POINTER *PageDirectoryPointerEntry; > + PAGE_TABLE_1G_ENTRY *PageDirectory1GEntry; > + PAGE_TABLE_ENTRY *PageDirectory2MEntry; > + PAGE_TABLE_4K_ENTRY *PageTableEntry; > + UINT64 PgTableMask; > + > + AddressEncMask = GetMemEncryptionAddressMask (); You unconditionally rewrite the static variable here, and then you rely on it in the other helper functions. What do you think about adding some internal caching to GetMemEncryptionAddressMask(), and then calling GetMemEncryptionAddressMask() explicitly everywhere the mask is needed? > + > + if (!AddressEncMask) { > + return RETURN_ACCESS_DENIED; > + } > + > + PgTableMask = AddressEncMask | EFI_PAGE_MASK; > + > + DEBUG ((EFI_D_VERBOSE, "Set memory range 0x%#Lx+0x%x (%s)\n", PhysicalAddress, Length, > + Mode == ClearCBit ? "unencrypted" : "encrypted")); Please use DEBUG_VERBOSE. Also, %x is not right for printing Length (UINT64). > + > + if (Length == 0) { > + return EFI_INVALID_PARAMETER; > + } > + > + // > + // We are going to change the memory encryption attribute from C=0 -> C=1 or vice versa > + // Flush the caches to ensure that data is written into memory with correct C-bit > + // > + WriteBackInvalidateDataCacheRange((VOID*) PhysicalAddress, Length); Please convert pointers to (UINTN) first. > + > + while (Length) > + { > + > + PageMapLevel4Entry = (VOID*) (AsmReadCr3() & ~PgTableMask); > + PageMapLevel4Entry += PML4_OFFSET(PhysicalAddress); > + if (!PageMapLevel4Entry->Bits.Present) { > + DEBUG((DEBUG_WARN, "ERROR bad PML4 for %lx\n", PhysicalAddress)); missing space after DEBUG > + return EFI_NO_MAPPING; > + } > + > + PageDirectory1GEntry = (VOID*) (PageMapLevel4Entry->Bits.PageTableBaseAddress<<12 & ~PgTableMask); missing spaces around << also, to clarify the binding precedence of << over &, please use parentheses. > + PageDirectory1GEntry += PDP_OFFSET(PhysicalAddress); > + if (!PageDirectory1GEntry->Bits.Present) { > + DEBUG((DEBUG_WARN, "ERROR bad PDPE for %lx\n", PhysicalAddress)); missing space after DEBUG... please check the rest of the occurrences > + return EFI_NO_MAPPING; > + } > + > + // If the MustBe1 bit is not 1, it's not actually a 1GB entry Comment style is // // comment // please check the rest > + if (PageDirectory1GEntry->Bits.MustBe1) { > + // Valid 1GB page > + // If we have at least 1GB to go, we can just update this entry > + if (!(PhysicalAddress & ((1<<30) - 1)) && Length >= (1<<30)) { For constants like the above, please consider using the macro BIT30 > + SetOrClearCBit(&PageDirectory1GEntry->Uint64, Mode); missing space after SetOrClearCBit > + DEBUG((DEBUG_VERBOSE, "Updated 1GB entry for %lx\n", PhysicalAddress)); > + PhysicalAddress += 1<<30; > + Length -= 1<<30; > + } else { > + // We must split the page > + DEBUG((DEBUG_VERBOSE, "Spliting 1GB page\n")); > + Split1GPageTo2M(((UINT64)PageDirectory1GEntry->Bits.PageTableBaseAddress)<<30, (UINT64*) PageDirectory1GEntry, 0, 0); > + continue; > + } > + } else { > + // Actually a PDP > + PageUpperDirectoryPointerEntry = (PAGE_MAP_AND_DIRECTORY_POINTER*) PageDirectory1GEntry; > + PageDirectory2MEntry = (VOID*) (PageUpperDirectoryPointerEntry->Bits.PageTableBaseAddress<<12 & ~PgTableMask); > + PageDirectory2MEntry += PDE_OFFSET(PhysicalAddress); > + if (!PageDirectory2MEntry->Bits.Present) { > + DEBUG((DEBUG_WARN, "ERROR bad PDE for %lx\n", PhysicalAddress)); > + return EFI_NO_MAPPING; > + } > + // If the MustBe1 bit is not a 1, it's not a 2MB entry > + if (PageDirectory2MEntry->Bits.MustBe1) { > + // Valid 2MB page > + // If we have at least 2MB left to go, we can just update this entry > + if (!(PhysicalAddress & ((1<<21)-1)) && Length >= (1<<21)) { > + SetOrClearCBit(&PageDirectory2MEntry->Uint64, Mode); > + DEBUG((DEBUG_VERBOSE, "Updated 2MB entry for %lx\n", PhysicalAddress)); > + PhysicalAddress += 1<<21; > + Length -= 1<<21; > + } else { > + // We must split up this page into 4K pages > + DEBUG((DEBUG_VERBOSE, "Spliting 2MB page at %lx\n", PhysicalAddress)); > + Split2MPageTo4K(((UINT64)PageDirectory2MEntry->Bits.PageTableBaseAddress) << 21, (UINT64*) PageDirectory2MEntry, 0, 0); > + continue; > + } > + } else { > + PageDirectoryPointerEntry = (PAGE_MAP_AND_DIRECTORY_POINTER*) PageDirectory2MEntry; > + PageTableEntry = (VOID*) (PageDirectoryPointerEntry->Bits.PageTableBaseAddress<<12 & ~PgTableMask); > + PageTableEntry += PTE_OFFSET(PhysicalAddress); > + if (!PageTableEntry->Bits.Present) { > + DEBUG((DEBUG_WARN, "ERROR bad PTE for %lx\n", PhysicalAddress)); > + return EFI_NO_MAPPING; > + } > + SetOrClearCBit(&PageTableEntry->Uint64, Mode); > + DEBUG((DEBUG_VERBOSE, "Updated 4KB entry for %lx\n", PhysicalAddress)); > + PhysicalAddress += EFI_PAGE_SIZE; > + Length -= EFI_PAGE_SIZE; > + } > + } > + } So, I didn't do any functional validation for the above, just coding style. > + > + // > + // Flush TLB > + // > + AsmWriteCr3(AsmReadCr3()); Please just call CpuFlushTlb(). > + return EFI_SUCCESS; > +} > + > +EFI_STATUS > +EFIAPI > +Set_Memory_Decrypted ( > + IN EFI_PHYSICAL_ADDRESS PhysicalAddress, > + IN UINT64 Length > + ) > +{ > + return Set_Memory_Enc_Dec (PhysicalAddress, Length, ClearCBit); > +} > + > +EFI_STATUS > +EFIAPI > +Set_Memory_Encrypted ( > + IN EFI_PHYSICAL_ADDRESS PhysicalAddress, > + IN UINT64 Length > + ) > +{ > + return Set_Memory_Enc_Dec (PhysicalAddress, Length, SetCBit); > +} > + > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h Can we upstream the contents of this file to MdePkg somehow? My first idea would be Include/IndustryStandard. However, the following types are already in "MdePkg/Include/Library/BaseLib.h": /// /// IA32 and x64 Specific Functions. /// Byte packed structure for 16-bit Real Mode EFLAGS. /// IA32_FLAGS16, IA32_EFLAGS32, IA32_CR0, IA32_CR4, IA32_SEGMENT_DESCRIPTOR, IA32_DESCRIPTOR, IA32_IDT_GATE_DESCRIPTOR, ... and a bunch of others. I'm asking because now we have: BaseTools/Source/C/GenPage/VirtualMemory.h DuetPkg/DxeIpl/Ia32/VirtualMemory.h DuetPkg/DxeIpl/X64/VirtualMemory.h MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h > new file mode 100644 > index 0000000..a556211 > --- /dev/null > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h > @@ -0,0 +1,158 @@ > +/** @file > + > + Virtual Memory Management Services to set or clear the memory encryption bit > + > + References: > + 1) IA-32 Intel(R) Architecture Software Developer's Manual Volume 1:Basic Architecture, Intel > + 2) IA-32 Intel(R) Architecture Software Developer's Manual Volume 2:Instruction Set Reference, Intel > + 3) IA-32 Intel(R) Architecture Software Developer's Manual Volume 3:System Programmer's Guide, Intel > + > +Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
> +Copyright (c) 2017, AMD Incorporated. All rights reserved.
> + > +This program and the accompanying materials > +are licensed and made available under the terms and conditions of the BSD License > +which accompanies this distribution. The full text of the license may be found at > +http://opensource.org/licenses/bsd-license.php > + > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#ifndef __VIRTUAL_MEMORY__ > +#define __VIRTUAL_MEMORY__ > + > +#include > +#include > +#include > +#include > +#include > +//#include > +//#include > + > +#include > +#define SYS_CODE64_SEL 0x38 > + > +#pragma pack(1) > + > +// > +// Page-Map Level-4 Offset (PML4) and > +// Page-Directory-Pointer Offset (PDPE) entries 4K & 2MB > +// > + > +typedef union { > + struct { > + UINT64 Present:1; // 0 = Not present in memory, 1 = Present in memory > + UINT64 ReadWrite:1; // 0 = Read-Only, 1= Read/Write > + UINT64 UserSupervisor:1; // 0 = Supervisor, 1=User > + UINT64 WriteThrough:1; // 0 = Write-Back caching, 1=Write-Through caching > + UINT64 CacheDisabled:1; // 0 = Cached, 1=Non-Cached > + UINT64 Accessed:1; // 0 = Not accessed, 1 = Accessed (set by CPU) > + UINT64 Reserved:1; // Reserved > + UINT64 MustBeZero:2; // Must Be Zero > + UINT64 Available:3; // Available for use by system software > + UINT64 PageTableBaseAddress:40; // Page Table Base Address > + UINT64 AvabilableHigh:11; // Available for use by system software > + UINT64 Nx:1; // No Execute bit > + } Bits; > + UINT64 Uint64; > +} PAGE_MAP_AND_DIRECTORY_POINTER; > + > +// > +// Page Table Entry 4KB > +// > +typedef union { > + struct { > + UINT64 Present:1; // 0 = Not present in memory, 1 = Present in memory > + UINT64 ReadWrite:1; // 0 = Read-Only, 1= Read/Write > + UINT64 UserSupervisor:1; // 0 = Supervisor, 1=User > + UINT64 WriteThrough:1; // 0 = Write-Back caching, 1=Write-Through caching > + UINT64 CacheDisabled:1; // 0 = Cached, 1=Non-Cached > + UINT64 Accessed:1; // 0 = Not accessed, 1 = Accessed (set by CPU) > + UINT64 Dirty:1; // 0 = Not Dirty, 1 = written by processor on access to page > + UINT64 PAT:1; // > + UINT64 Global:1; // 0 = Not global page, 1 = global page TLB not cleared on CR3 write > + UINT64 Available:3; // Available for use by system software > + UINT64 PageTableBaseAddress:40; // Page Table Base Address > + UINT64 AvabilableHigh:11; // Available for use by system software > + UINT64 Nx:1; // 0 = Execute Code, 1 = No Code Execution > + } Bits; > + UINT64 Uint64; > +} PAGE_TABLE_4K_ENTRY; > + > +// > +// Page Table Entry 2MB > +// > +typedef union { > + struct { > + UINT64 Present:1; // 0 = Not present in memory, 1 = Present in memory > + UINT64 ReadWrite:1; // 0 = Read-Only, 1= Read/Write > + UINT64 UserSupervisor:1; // 0 = Supervisor, 1=User > + UINT64 WriteThrough:1; // 0 = Write-Back caching, 1=Write-Through caching > + UINT64 CacheDisabled:1; // 0 = Cached, 1=Non-Cached > + UINT64 Accessed:1; // 0 = Not accessed, 1 = Accessed (set by CPU) > + UINT64 Dirty:1; // 0 = Not Dirty, 1 = written by processor on access to page > + UINT64 MustBe1:1; // Must be 1 > + UINT64 Global:1; // 0 = Not global page, 1 = global page TLB not cleared on CR3 write > + UINT64 Available:3; // Available for use by system software > + UINT64 PAT:1; // > + UINT64 MustBeZero:8; // Must be zero; > + UINT64 PageTableBaseAddress:31; // Page Table Base Address > + UINT64 AvabilableHigh:11; // Available for use by system software > + UINT64 Nx:1; // 0 = Execute Code, 1 = No Code Execution > + } Bits; > + UINT64 Uint64; > +} PAGE_TABLE_ENTRY; > + > +// > +// Page Table Entry 1GB > +// > +typedef union { > + struct { > + UINT64 Present:1; // 0 = Not present in memory, 1 = Present in memory > + UINT64 ReadWrite:1; // 0 = Read-Only, 1= Read/Write > + UINT64 UserSupervisor:1; // 0 = Supervisor, 1=User > + UINT64 WriteThrough:1; // 0 = Write-Back caching, 1=Write-Through caching > + UINT64 CacheDisabled:1; // 0 = Cached, 1=Non-Cached > + UINT64 Accessed:1; // 0 = Not accessed, 1 = Accessed (set by CPU) > + UINT64 Dirty:1; // 0 = Not Dirty, 1 = written by processor on access to page > + UINT64 MustBe1:1; // Must be 1 > + UINT64 Global:1; // 0 = Not global page, 1 = global page TLB not cleared on CR3 write > + UINT64 Available:3; // Available for use by system software > + UINT64 PAT:1; // > + UINT64 MustBeZero:17; // Must be zero; > + UINT64 PageTableBaseAddress:22; // Page Table Base Address > + UINT64 AvabilableHigh:11; // Available for use by system software > + UINT64 Nx:1; // 0 = Execute Code, 1 = No Code Execution > + } Bits; > + UINT64 Uint64; > +} PAGE_TABLE_1G_ENTRY; > + > +#pragma pack() > + > +#define IA32_PG_P BIT0 > +#define IA32_PG_RW BIT1 > + > +#define PAGETABLE_ENTRY_MASK ((1UL << 9) - 1) > +#define PML4_OFFSET(x) ( (x >> 39) & PAGETABLE_ENTRY_MASK) > +#define PDP_OFFSET(x) ( (x >> 30) & PAGETABLE_ENTRY_MASK) > +#define PDE_OFFSET(x) ( (x >> 21) & PAGETABLE_ENTRY_MASK) > +#define PTE_OFFSET(x) ( (x >> 12) & PAGETABLE_ENTRY_MASK) > +#define PAGING_1G_ADDRESS_MASK_64 0x000FFFFFC0000000ull > + > +EFI_STATUS > +EFIAPI > +Set_Memory_Decrypted ( > + IN EFI_PHYSICAL_ADDRESS PhysicalAddress, > + IN UINT64 Length > + ); > + > +EFI_STATUS > +EFIAPI > +Set_Memory_Encrypted ( > + IN EFI_PHYSICAL_ADDRESS PhysicalAddress, > + IN UINT64 Length > + ); > + > +#endif > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index 769251d..c2821b7 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -126,6 +126,7 @@ > QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf > VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf > LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf > + MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf > !if $(SMM_REQUIRE) == FALSE > LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > !endif > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index 3874c35..1dd8064 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -131,6 +131,7 @@ > QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf > VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf > LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf > + MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf > !if $(SMM_REQUIRE) == FALSE > LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > !endif > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index fe7f086..06bee32 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -131,6 +131,7 @@ > QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf > VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf > LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf > + MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf > !if $(SMM_REQUIRE) == FALSE > LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > !endif > Thanks Laszlo