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 C796A20D2C3BA for ; Mon, 27 Mar 2017 01:23:20 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2BF348553F; Mon, 27 Mar 2017 08:23:20 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 2BF348553F Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 2BF348553F 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 191397754D; Mon, 27 Mar 2017 08:23:17 +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> <149013078089.27235.18195163049694122262.stgit@brijesh-build-machine> Cc: brijesh.singh@amd.com, leo.duran@amd.com, Thomas.Lendacky@amd.com From: Laszlo Ersek Message-ID: <17bcbc2f-d4b9-4c7b-c6f9-9ae157ef93ca@redhat.com> Date: Mon, 27 Mar 2017 10:23:16 +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: <149013078089.27235.18195163049694122262.stgit@brijesh-build-machine> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Mon, 27 Mar 2017 08:23:20 +0000 (UTC) Subject: Re: [RFC PATCH v2 03/10] OvmfPkg/PlatformPei: Add Secure Encrypted Virutualization (SEV) support 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 08:23:21 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Please fix the typo ("Virutualization") in the subject. On 03/21/17 22:13, Brijesh Singh wrote: > Initialize Secure Encrypted Virtualization support and set the memory encryption mask PCD. Please wrap commit messages at 74 characters. > > Signed-off-by: Brijesh Singh > --- > OvmfPkg/OvmfPkgIa32.dsc | 3 + > OvmfPkg/OvmfPkgIa32X64.dsc | 3 + > OvmfPkg/OvmfPkgX64.dsc | 3 + > OvmfPkg/PlatformPei/AmdSev.c | 97 +++++++++++++++++++++++++++++++++++ > OvmfPkg/PlatformPei/Platform.c | 1 > OvmfPkg/PlatformPei/Platform.h | 5 ++ > OvmfPkg/PlatformPei/PlatformPei.inf | 2 + > 7 files changed, 114 insertions(+) > create mode 100644 OvmfPkg/PlatformPei/AmdSev.c > > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index 546cdf7..769251d 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -506,6 +506,9 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 > gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 > > + # Set memory encryption mask > + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0 > + Please update your git config as described here: https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05 (the "xfuncname" setting in particular,) and here: https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09 The point of these settings is that the diff hunk header marked with @@ above will display the DSC section that the hunk modifies (and I'll know immediately that you are adding a dynamic PCD default). > !if $(SMM_REQUIRE) == TRUE > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index 383c8d3..3874c35 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -514,6 +514,9 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 > gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 > > + # Set memory encryption mask > + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0 > + > !if $(SMM_REQUIRE) == TRUE > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 0b7533c..fe7f086 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -513,6 +513,9 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 > gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 > > + # Set memory encryption mask > + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0 > + > !if $(SMM_REQUIRE) == TRUE > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 > diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c > new file mode 100644 > index 0000000..7f05a9a > --- /dev/null > +++ b/OvmfPkg/PlatformPei/AmdSev.c New file -- can you please double check it is CRLF terminated? Hm, looking at your "sev-rfc-2" branch, the file does use CRLF. Good. > @@ -0,0 +1,97 @@ > +/**@file > + Initialize Secure Encrypted Virtualization (SEV) support > + > + 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 This line is too wide. Please make sure no line is wider than 79 chars. Please check your patches with python BaseTools/Scripts/PatchCheck.py before submission. > + 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. > + > +**/ > +// > +// The package level header files this module uses > +// > +#include > + > +#include > +#include > +#include > +#include > + > +/** > + > + Function returns 'TRUE' when SEV is enabled otherwise FALSE > + > + **/ One idiomatic way to write this comment is: /** Query whether SEV is enabled. @retval TRUE SEV is enabled. @retval FALSE Otherwise. **/ > +STATIC > +BOOLEAN > +SevIsEnabled ( > + VOID > + ) > +{ > + UINT32 RegEax; > + MSR_SEV_STATUS_REGISTER Msr; > + CPUID_MEMORY_ENCRYPTION_INFO_EAX Eax; Please align the definitions like this: UINT32 RegEax; MSR_SEV_STATUS_REGISTER Msr; CPUID_MEMORY_ENCRYPTION_INFO_EAX Eax; > + > + // > + // 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; > + } > + } > + } > + > + return FALSE; > +} > + > +/** > + Function checks if SEV support is available, if present then it updates > + the dynamic PcdPteMemoryEncryptionAddressOrMask with memory encryption mask. > + > + **/ > +VOID > +EFIAPI > +AmdSevInitialize ( > + VOID > + ) > +{ > + UINT64 MeMask; > + CPUID_MEMORY_ENCRYPTION_INFO_EBX Ebx; Same alignment / layout comment. Also, I suggest renaming "MeMask" to "EncryptionMask". > + > + // > + // Check if SEV is enabled > + // > + if (!SevIsEnabled ()) { > + return; > + } > + > + // > + // 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); > + > + // > + // Set Memory Encryption Mask PCD > + // > + PcdSet64S (PcdPteMemoryEncryptionAddressOrMask, MeMask); Please save the status of the PcdSet64S() function call, and assert that it works: RETURN_ERROR PcdStatus; PcdStatus = PcdSet64S (...); ASSERT_RETURN_ERROR (PcdStatus); > + > + DEBUG ((EFI_D_INFO, "SEV support is enabled (mask 0x%lx)\n", MeMask)); We no longer use EFI_D_* macros in new code, please use DEBUG_* instead. > +} > diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c > index 77a8a16..49e6c66 100644 > --- a/OvmfPkg/PlatformPei/Platform.c > +++ b/OvmfPkg/PlatformPei/Platform.c > @@ -667,6 +667,7 @@ InitializePlatform ( > NoexecDxeInitialization (); > } > > + AmdSevInitialize (); > MiscInitialization (); > InstallFeatureControlCallback (); > OK, so this is something we do on S3 resume as well... Yes, S3Resume2Pei consumes this PCD. > diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h > index 18f42c3..a7729b9 100644 > --- a/OvmfPkg/PlatformPei/Platform.h > +++ b/OvmfPkg/PlatformPei/Platform.h > @@ -88,6 +88,11 @@ XenDetect ( > VOID > ); > > +VOID > +AmdSevInitialize ( > + VOID > + ); > + > extern BOOLEAN mXen; > > VOID Again, I request that you please configure your edk2 working tree as described in the above wiki article. In particular, https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-10 and https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23 will ensure that .h files are formatted before .c files into the patches, which helps quite a bit with review. (With newer git, you can set -O permanently for git-format-patch, through "diff.orderFile".) > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf > index 53c6dd4..2cf4ac876 100644 > --- a/OvmfPkg/PlatformPei/PlatformPei.inf > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf > @@ -35,6 +35,7 @@ > MemDetect.c > Platform.c > Xen.c > + AmdSev.c > > [Packages] If possible please try to keep the list of files alphabetically sorted. > IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec > @@ -98,6 +99,7 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber > gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize > + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask Ditto for the list of PCDs. Functionally the patch looks okay to me. Thanks, Laszlo > > [FixedPcd] > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >