From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-x234.google.com (mail-qt0-x234.google.com [IPv6:2607:f8b0:400d:c0d::234]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7DED821BC6A7E for ; Mon, 27 Mar 2017 05:22:10 -0700 (PDT) Received: by mail-qt0-x234.google.com with SMTP id r45so35237542qte.3 for ; Mon, 27 Mar 2017 05:22:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=3ejz5M1PmH/tHLi2KNNbMtCn/CSnSeGtyNd0ddHvMjg=; b=fluNq2AlUuZVejUauQwi5JKxabbqusC/qw2GTp+XYEsd/KM9cv3Y8qcd2bObmITeDK EVOtCh68Ldy1ndVZBnjy7ArnOkbFSrXrRobHAqh9J/brQO+OgpJ/Whdv0B8JFHjVQVWp vfGg7HV1lY8J2UQAgi4YeJaUQ9Qpi0fxkA/6t3gNwFqAkQzo3Uszc8u9IPCE95S0kH69 /r+bYap6oN8rbBA2mPfKqh60Tf3p5T4tuNtDiYLrDF0GQxuI2cRlLyaGj4tvosNVbspX /qrE/qENnGc04mwzmXUq8anZz7IIEWsx4ka2xQIPMMe/68Jd+KmrVXdNW8Zm1jvGYRm1 KlXg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=3ejz5M1PmH/tHLi2KNNbMtCn/CSnSeGtyNd0ddHvMjg=; b=BZcx8+QEN6Ztm0/NofmlI+FBawEmpMhxsgyyTbJQXAWsyBXwXfTtTBqm1J1/asybpT /bi1LoK9VHuXndM4gBJSc5AOQ4HGBgL6HRo9P+5ljJJgq8L0o4SH1e9qEwYvzSoITGd/ JDPHRGp7Za4bCzkLEZKWt2tPAYCeMb0VNn+4g7Sb64mtbr2ovhyWDardlSq5DW974NnG fRobvbm1dxh22xTCg7RvY133d4CTKYGGnWw0rC3OWZGm42cvnd/8WgJGnyVwZBO5hcyr XdiqrWJeCRI40Q99m+2hWHLc52nPzQ4he4itZC+v1iU7Yo1UEEfXaogQKsvrjirdSp7t YNcQ== X-Gm-Message-State: AFeK/H2VPnq1l5bOdpJl65OQce3w45yfehBm0K6oyWoVeMhpmBIBkcdqp/+PhhgSrp289J2vmrFIRVhf9Ye7hA== X-Received: by 10.200.47.161 with SMTP id l30mr20793074qta.248.1490617329497; Mon, 27 Mar 2017 05:22:09 -0700 (PDT) MIME-Version: 1.0 Received: by 10.12.182.65 with HTTP; Mon, 27 Mar 2017 05:22:09 -0700 (PDT) In-Reply-To: <17bcbc2f-d4b9-4c7b-c6f9-9ae157ef93ca@redhat.com> References: <149013076154.27235.10725020825643505862.stgit@brijesh-build-machine> <149013078089.27235.18195163049694122262.stgit@brijesh-build-machine> <17bcbc2f-d4b9-4c7b-c6f9-9ae157ef93ca@redhat.com> From: Brijesh Singh Date: Mon, 27 Mar 2017 07:22:09 -0500 Message-ID: To: Laszlo Ersek Cc: "Kinney, Michael D" , "Justen, Jordan L" , edk2-devel@ml01.01.org, "Gao, Liming" , brijesh.singh@amd.com, Leo Duran , Tom Lendacky X-Content-Filtered-By: Mailman/MimeDel 2.1.22 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 12:22:10 -0000 Content-Type: text/plain; charset=UTF-8 On Mon, Mar 27, 2017 at 3:23 AM, Laszlo Ersek wrote: > 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. > > Will do. > > > > 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.PcdPteMemoryEncryptionAddressO > rMask|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). > > Thanks for the pointer, I will configure our project based on the settings described in the wiki. > > !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.PcdPteMemoryEncryptionAddressO > rMask|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.PcdPteMemoryEncryptionAddressO > rMask|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. > **/ > > I will update comment. > > +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; > > > Will do > > + > > + // > > + // 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". > > Will fix both alignment/layout and rename the variable. > > + > > + // > > + // 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); > > Okay, will update the code. > > + > > + 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. > > Ah, I was not aware of this. I may have used this macro in other patches. I will go and fix them. > > +} > > 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. > > I did run a S3 resume and could see the code was kicking in and we were able to resume fine. > > 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".) > Sure I will update gitconfig file with those settings. > 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. > > Will do. > > 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 > Thanks, > Laszlo > > > > > [FixedPcd] > > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel > > > > -- Confusion is always the most honest response.