On 10/2/19 7:30 AM, Laszlo Ersek via Groups.Io wrote: > On 10/02/19 14:24, Laszlo Ersek wrote: >> On 09/19/19 21:52, Lendacky, Thomas wrote: >>> From: Tom Lendacky <thomas.lenda...@amd.com> >>> >>> 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 <jordan.l.jus...@intel.com> >>> Cc: Laszlo Ersek <ler...@redhat.com> >>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >>> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com> >>> --- >>> 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().
Yup, it was just for the cache checking. I'll do the open-coding. Thanks, Tom > > 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 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48382): https://edk2.groups.io/g/devel/message/48382 Mute This Topic: https://groups.io/mt/34203552/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
