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().
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 (#48370): https://edk2.groups.io/g/devel/message/48370
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]
-=-=-=-=-=-=-=-=-=-=-=-
