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:24:20 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B4623300BEAD; Wed, 2 Oct 2019 12:24:19 +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 EF73860BE0; Wed, 2 Oct 2019 12:24:16 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v2 16/44] OvmfPkg/MemEncryptSevLib: Make MemEncryptSevLib available during SEC 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: From: "Laszlo Ersek" Message-ID: Date: Wed, 2 Oct 2019 14:24:15 +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.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Wed, 02 Oct 2019 12:24:19 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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. Thanks, Laszlo