From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 02 Oct 2019 05:30:17 -0700 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 67819A44AFD; Wed, 2 Oct 2019 12:30:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-71.rdu2.redhat.com [10.10.120.71]) by smtp.corp.redhat.com (Postfix) with ESMTP id B96665C224; Wed, 2 Oct 2019 12:30:13 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v2 16/44] OvmfPkg/MemEncryptSevLib: Make MemEncryptSevLib available during SEC From: "Laszlo Ersek" To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , "Singh, Brijesh" References: Message-ID: <0c90a055-3732-cffa-7b76-866eb09e5bd7@redhat.com> Date: Wed, 2 Oct 2019 14:30:12 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.68]); Wed, 02 Oct 2019 12:30:16 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/02/19 14:24, Laszlo Ersek wrote: > On 09/19/19 21:52, Lendacky, Thomas wrote: >> From: Tom Lendacky >> >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 >> >> The SEC phase of OVMF will need access to the MemEncryptSevLib library, >> so make the library available during SEC. >> >> Cc: Jordan Justen >> Cc: Laszlo Ersek >> Cc: Ard Biesheuvel >> Signed-off-by: Tom Lendacky >> --- >> OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf >> index 7c44d0952815..755d49cc22dc 100644 >> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf >> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf >> @@ -14,7 +14,7 @@ [Defines] >> 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 >> + LIBRARY_CLASS = MemEncryptSevLib|SEC PEIM DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_DRIVER >> >> # >> # The following information is for reference only and not required by the build >> > > This is not a good idea, at least in this form. > > This library instance uses multiple variables with static storage > duration, such as: > - mSevStatus > - mSevEsStatus > - mSevStatusChecked > - mAddressEncMaskChecked [X64] > - mAddressEncMask [X64] > - mPageTablePool [X64] > > SEC runs from pflash, so such variables are read-only (writes to them > won't trap, but will have no effect). > > What are the functions from "OvmfPkg/Include/Library/MemEncryptSevLib.h" > that you need in the SEC phase? > > Because, maybe we should split the library class in two classes. One lib > class would declare (for example): > - MemEncryptSevEsIsEnabled > - MemEncryptSevIsEnabled > > The other lib class would declare: > - MemEncryptSevClearPageEncMask > - MemEncryptSevSetPageEncMask > - MemEncryptSevLocateInitialSmramSaveStateMapPages > > The first lib class could have two instances, one for SEC (using no > global variables for caching / speeding up the checks), and another > instance for PEI and later (with the global variables used for caching > the detection results). > > The second lib class would have only one instance (the current one), and > it would not be usable in the SEC phase. (The main issue is that, for > manipulating PTEs, MemEncryptSevSetPageEncMask and > MemEncryptSevClearPageEncMask sometimes need to split page tables, and > for that, they need to allocate pages dynamically. We can't do that in SEC.) > > The [LibraryClasses] sections of some INF files that currently list > "MemEncryptSevLib" may have to be updated, corresponding to the split. > > Again, all of this depends on the exact subset of APIs you need in SEC. > If it's just one API, e.g. MemEncryptSevEsIsEnabled(), needed in just > one place in SEC, then it might not necessarily be tragic to simply > open-code (duplicate) the CPUID logic in SEC. Ah right, so if it's just for patch#18 ("OvmfPkg/Sec: Enable cache early to speed up booting"), then I'd definitely opt for open-coding the two AsmCpuid(), plus one AsmReadMsr32(), calls, in SecCoreStartupWithStack(). Later on, *if* we decide to call AsmEnableCache() in SecCoreStartupWithStack() unconditionally -- and that's a big "if" --, then it's going to be easier to remove those CPUID+MSR checks, than to undo the MemEncryptSevLib class split as well. Thanks Laszlo